Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exporter README, receiver README updates #29

Merged
merged 9 commits into from
Aug 31, 2023

Conversation

jmacd
Copy link
Contributor

@jmacd jmacd commented Aug 29, 2023

This flushes out the exporter README and makes a few corresponding fixes to the receiver README.

Copy link
Contributor

@moh-osman3 moh-osman3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - found some typos and just had some basic questions for my understanding

Comment on lines 146 to 147
- `recevier_recv`: uncompressed bytes received, prior to compression
- `recevier_recv_wire`: compressed bytes received, on the wire.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/recevier_recv/receiver_recv

s/recevier_recv_wire/receiver_recv_wire

collector/receiver/otelarrowreceiver/README.md Outdated Show resolved Hide resolved
collector/exporter/otelarrowexporter/README.md Outdated Show resolved Hide resolved
collector/exporter/otelarrowexporter/README.md Outdated Show resolved Hide resolved
collector/exporter/otelarrowexporter/README.md Outdated Show resolved Hide resolved
Comment on lines 94 to 95
- `disabled`: disables use of Arrow, causing the exporter to use standard OTLP
- `disable_downgrade`: prevents this exporter from using standard OTLP
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we mention what the default value is for these settings?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

collector/exporter/otelarrowexporter/metadata.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@lquerel lquerel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jmacd
Copy link
Contributor Author

jmacd commented Aug 31, 2023

@moh-osman3 FYI I observed one local test flake on TestReceiverEOF which seems reproducible locally. Since the tests pass here, I'm going with it.

@jmacd jmacd merged commit fe5a5a5 into open-telemetry:main Aug 31, 2023
1 check passed
@jmacd jmacd deleted the jmacd/exp_readme branch August 31, 2023 16:49
jmacd added a commit that referenced this pull request Aug 31, 2023
Here at Lightstep we think that round_robin is a reasonably good default
load balancer for OTel Arrow, while `pick_first` is what we get without
setting something in this field. `pick_first` is not a reasonably good
default load balancer for OTel Arrow. This change was documented in #29
already.

Fixes #32.
jmacd pushed a commit that referenced this pull request Aug 31, 2023
After merging #24, unit
tests for otelarrowreceiver were failing and I did not catch that. This
PR fixes the unit tests by adding in support for an extra call to Send()
the receiver will do (with arrowpb.StatusCode_STREAM_SHUTDOWN), to
signal shutdown to the client.

Addresses
#29 (comment)
@jmacd jmacd mentioned this pull request Aug 31, 2023
jmacd added a commit that referenced this pull request Aug 31, 2023
Release v0.3.0 includes:
- #24
- #29 
- #30 
- #36 

This release includes a BREAKING CHANGE 🛑 🛑 🛑 🛑 🛑.
We do not anticipate another of these, it merely renames/enumbers status
codes to match the gRPC status code numbering scheme. However, since
status code 0 stays the same, it will result in unusual looking errors
but should be safe to deploy the receivers first. Their CANCELED (i.e.,
SERVER_SHUTDOWN) will read as UNAVAILABLE, etc.

See #27.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants